Conversation
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned. One non-blocking note inline about the log_path suffix not reaching the user in the multi-line case.
What I checked
- The diff itself, plus the callers on both sides:
_build_tui_agent_loop(raven/cli/tui_commands.py:483-525) is the only producer of theseInternalErrors, and it always passesdata, which is exactly the case the old dispatcher droppeddetailfor. The consumer of the new event field isonErrorinui-tui/src/app/chatStream.ts:210-224, which is live (wired throughcreateChatStreaminuseMainApp.ts), so the server half is not dead code. - Schema:
ErrorEventPayload.detailalready exists inraven/tui_rpc/models.py:212-216and in the checked-inui-tui/src/rpc/generated.ts, so nothing needed regenerating and the strict payload model still accepts the emitted dict. - Backward compatibility: both new fields are additive. An older client ignores the extra
detailkey; a newer client against an older server just formats the bare code name, which is what the first test inrpc.test.tspins. error_datasemantics:setdefaultmeans a raiser that sets bothdata["detail"]andexc.detailkeeps thedataone, anddata or Nonenow omitserror.datawhen a raiser passes an empty dict. Neither has a caller in the tree, and no consumer requiresdatato be present, so I am not raising them.- Project rules: commit header follows Conventional Commits with a real scope, the whole message is ASCII (
git log -1 --format=%B | grep -nP "[^\x00-\x7F]"prints nothing), no new test files were created where an existing one exists (AGENTS.md 5.4), no assets. - Tests were not weakened. Both new Python tests assert real behaviour, and the negative one (
..._omits_detail_when_the_build_error_has_no_cause) is a genuine assertion, not a skip. The TSignores non-object and blank datacase loops over six hostile shapes rather than asserting one.
What I ran
uv run pytest tests -k tui -q
611 passed, 7 skipped, 5335 deselected in 40.42s
The 7 skips are the pre-existing optional-channel import guards (dingtalk_stream, lark_oapi, nio, botpy, slack_sdk, telegram, wecom_aibot_sdk) and are unrelated to this change.
cd ui-tui && npx vitest run
Test Files 86 passed (86)
Tests 988 passed (988)
npx tsc --noEmit # clean
uv run ruff check raven/tui_rpc tests/test_tui_rpc_system.py tests/test_tui_rpc_turn_send.py
All checks passed!
(The suite needs packages/hermes-ink built first -- npx esbuild src/entry-exports.ts --bundle --platform=node --format=esm --packages=external --outdir=dist -- otherwise 41 files fail on a missing ./dist/entry-exports.js. That is an environment prerequisite, not this branch.)
Nit, take it or leave it
The doc comment at ui-tui/src/rpc/errors.ts:21 writes the example as (see ~/.raven/logs/tui.log) but the code emits (details in ...).
A -32603 reached the user as the bare line `error: [rpc -32603]
internal_error`, with nothing about what failed or where to look. The cause
was collected and then thrown away three times over:
- the dispatcher dropped `RpcError.detail` whenever the raiser also passed
`data`, which `_build_tui_agent_loop` always does;
- `turn.send` emitted a latched init crash as `{code, message}` only, even
though the ErrorEvent schema already has a `detail` field the front end
renders;
- the client formatted `[rpc <code>] <message>`, where `message` is a fixed
code name, and every call site prints `err.message`.
So an AgentLoop that cannot start (a config the running branch cannot parse,
for instance) produced an error that named neither the file nor the log.
Fold `detail` into `error.data` in a shared `error_data` helper used by both
the dispatcher and the turn events, pass the init-crash detail plus its
`log_path` into the emitted event, and build the client-side message from
`detail` / `exception_message` / `reason` with the log path appended. A
multi-line cause (a config error listing each offending field) keeps its
line breaks below the summary line.
Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
Review caught that the pointer did not survive to the user in the case that motivated it. The transcript renders the first line of `detail` and nothing more (ui-tui/src/app/chatStream.ts), and the crash this detail exists for -- a config the running build cannot parse -- raises a ValidationError whose str() is multi-line: 7 lines for two bad fields. Appended, `(details in ~/.raven/logs/tui.log)` landed on the last line, so the user saw `2 validation errors for RavenConfig` and no path. Verified before changing anything. Prepend it instead. Single-line causes read the same either way, and the multi-line ones now keep both halves in the rendered slice. The existing test asserted the whole string, which a trailing pointer passes just as well; the new one asserts what the renderer actually keeps. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
b6b8ab4 to
e8417b2
Compare
|
Claude finished @arelchan's task in 3m 0s —— View job ReviewNo blockers; this can merge.
What's new since gloryfromca's reviewThe second commit (
The doc-comment nit gloryfromca flagged ( ConventionsBoth commit headers are Conventional Commits with a real scope, ASCII-only ( TestsNew/changed tests genuinely assert behavior (not weakened): the dispatcher test asserts Note: I could not execute |
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Re-reviewing only the delta since b6b8ab48, which is where I left off. The first commit is byte-identical after the rebase (diff <(git show b6b8ab4851b7 --format="") <(git show 9c7e8797 --format="") is empty), so the substance I already reviewed has not moved and I am not repeating it. The new commit e8417b22 is the one thing I asked for and nothing else: the log pointer moved to the front of detail, and the existing whole-string assertion was updated rather than relaxed, with a second test added that slices detail the way chatStream.ts does -- so the shape that failed before now fails the suite instead of passing it. That is the right place to put the assertion; the previous one passed a trailing pointer just as happily, which is exactly how the gap got in.
On the delta I checked: that the renderer this is calibrated against is still the same after the rebase (ui-tui/src/app/chatStream.ts:250, moved from :221 by main's changes, logic unchanged); that no other test or source pins the old appended order (grep -rn "details in" finds only the new Python string, its test, and the TS frame path, which puts the pointer on its own line and is unaffected); that the 200-char slice still leaves ~165 chars for the cause after the ~35-char pointer; and that the commit message is ASCII with a real scope.
uv run pytest tests -k tui -q
617 passed, 7 skipped, 5827 deselected in 25.57s
cd ui-tui && npx vitest run
Test Files 86 passed (86)
Tests 990 passed (990)
uv run ruff check raven/tui_rpc tests/test_tui_rpc_turn_send.py
All checks passed!
Count went 611 -> 617: one is the new test here, the rest came in with the rebase onto main. The 7 skips are the same pre-existing optional-channel import guards.
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The new commit fixes the prior truncation gap: the streamed error now puts the log pointer before a multiline ValidationError, and the regression test asserts the same first-line, 200-character slice used by chatStream. I found no code issue worth raising.
I covered the full diff, the new delta, the startup-error source, dispatcher and streamed-error callers, client reconstruction and transcript rendering, history, backward compatibility of typed RPC errors and raw data, test strength, and the AGENTS.md/CONTEXT architecture and naming constraints.
Verification:
uv run pytest tests -k tui -q: 617 passed, 7 skipped, 5827 deselected.cd ui-tui && npm test: 86 files passed; 977 passed, 13 skipped (990 total).cd ui-tui && npm run type-check && npm run lint:rpc: passed; generated RPC types are in sync.git diff --check github/main...HEAD: passed.
Integration note: GitHub currently reports this head as CONFLICTING because current main renamed raven/tui_rpc to raven/rpc and the matching tests. The patch therefore needs a mechanical rebase/port before GitHub can merge it, followed by review of that new revision. I am not treating target-branch drift as a late-round product blocker because it is not introduced behavior and it has a direct author-side resolution.
I also resolved the earlier log-path thread after confirming its fix at this head.
3 participants
Summary
A -32603 reached the user as exactly this, and nothing else:
No cause, no file, no log path. The information existed the whole time and was discarded
three separate times:
detail.raven/tui_rpc/dispatcher.pyusedif exc.data is not None: ... elif exc.detail: ..., so a raiser that set both lost thedetail.
_build_tui_agent_loopalways sets both.turn.sendemitted only{code, message}for a latched init crash, although theErrorEventschema already has adetailfield andchatStream.onErroralready rendersit.
[rpc <code>] <message>, wheremessageis a fixed code name(
internal_error), and every call site printserr.message.So an AgentLoop that cannot start produced a dead end. A real instance: a config file
containing a key the running branch does not know about fails validation,
_build_tui_agent_loopraises
InternalErrorwith the full pydantic message andlog_path, and the TUI showed theline above.
The fix keeps
messageprotocol-faithful and puts the cause where callers already look:error_data(exc)inraven/tui_rpc/errors.pyfoldsdetailintodata, and both thedispatcher and the turn error events use it, so the two paths carry the same context.
turn.sendpasses the init-crash detail plus itslog_pathinto the emitted event.formatRpcErrorinui-tui/src/rpc/errors.tsbuilds the message fromdetail,exception_messageorreason, and appends the log path. A multi-line cause (a configerror listing each offending field) keeps its line breaks below the summary line.
Same failure after the change, rendered by feeding a real dispatcher frame through the client
path:
Type
Verification
uv run pytest tests/ -k "tui_rpc or dispatcher or tui_bootstrap"-> 523 passed. New cases:the dispatcher keeps
detailnext toreasonandlog_pathin one frame;turn.sendemitsthe build-error cause with its log path; and it omits
detailentirely when the error has nocause to report.
npx vitest runinui-tui/-> 86 files, 988 tests passed, including six newrpcErrorFromFramecases: a bare frame keeps the old one-line form;datacontext issurfaced with the log path;
exception_messageandreasonare read whendetailis absent;a multi-line cause keeps its shape; non-object or blank
datacannot corrupt the message; andtyped subclass selection plus raw
dataaccess are unchanged.npm run type-check,npx eslint,npx prettier --check,ruff check,ruff format --checkall clean.
End to end, the exact production shape: an
InternalErrorraised the way_build_tui_agent_loopraises it was dispatched to a JSON-RPC frame, and that frame was fedthrough
rpcErrorFromFramewithtsx, producing the output shown above.Risk
Notes:
error.codeanderror.messageare unchanged on the wire, so nothing that branches onthem is affected; the added
data.detailandpayload.detailare both already in the schema.err.messagegains a suffix, which the one message-matching call site(
SESSION_BUSY_REinuseSubmission.ts) is unaffected by, since it matches text that stillappears. Detail text originates from server-side exceptions and is already written to
~/.raven/logs/tui.log, so this exposes nothing new to the client; the client accepts it onlywhen it is a string and ignores any other shape. Rollback is a revert of this commit.
Related Issues
N/A